fix: Inject component annotations into HTML elements rather than React components #848
Conversation
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Bug Fixes 🐛
🤖 This preview updates automatically when you update the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
timfish
force-pushed
the
timfish/fix/inject-annotations-into-html-elements
branch
2 times, most recently
from
729d328 to
d498cc1
Compare
timfish
force-pushed
the
timfish/fix/inject-annotations-into-html-elements
branch
2 times, most recently
from
32c96e7 to
148b620
Compare
timfish
changed the title
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks! I'll let other folks also review, especially also get perspective from ReactNative so that we're on the same page.
There was a problem hiding this comment.
super-l: Should we refactor the tests to use toMatchInlineSnapshot? Feels way easier to me to associate which test corresponds to which snapshot. Also fine to tackle as a follow-up or not at all if I'm missing a reason for keeping it as-is. WDYT?
packages/babel-plugin-component-annotate/test/__snapshots__/test-plugin.test.ts.snap
Outdated
Show resolved
Hide resolved
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
LGTM but as discussed offline, we can put this behind a flag to reduce bundle size impact
timfish
force-pushed
the
timfish/fix/inject-annotations-into-html-elements
branch
from
a03a02a to
615a4ef
Compare
| "@react-navigation", | ||
| ]; | ||
|
|
||
| export const REACT_NATIVE_ELEMENTS: string[] = [ |
There was a problem hiding this comment.
I think there are missing elements here (e.g. Pressable, TouchableOpacity that we are using in our samples). I'm not sure we can include everything in this list tbh. The users may also add custom components in their apps.
I was wondering if we can make this configurable so that we can add more elements on the RN SDK side or even allow for end users to extend.
antonis
left a comment
antonis
left a comment
There was a problem hiding this comment.
As long as we do not pass experimentalInjectIntoHtml: true from the React Native SDK my understanding is that there is no change in behavior and the changes are safe to ship.
We can experiment with turning the feature ON and see the impact on our side. I think the main issue is the missing components before making this behavior default on the RN SDK.
|
Closing in favour of #851 |
4 participants
Rather than inject attributes into React components which can cause incompatibilities with props, this PR changes the plugin to instead inject attributes into every HTML element in the component root.
Downsides
Potentially larger bundle sizes because the attributes will get injected into more elements.